Add automatic column synchronization for Excel-to-table syncs by nabilbotpress · Pull Request #210 · botpress/growth

nabilbotpress · 2025-10-31T21:30:21Z

No description provided.

ermek-botpress · 2025-11-03T14:27:15Z

integrations/sharepoint-excel/src/index.ts


  actions: {
-    syncExcelFile: async ({ ctx, input, logger }) => {
+    syncExcelFile: async ({ ctx, input, logger, client }) => {


issue: Could you separate the action implementation into a separate file. Refer to other integrations

ermek-botpress · 2025-11-03T14:29:37Z

integrations/sharepoint-excel/src/index.ts

+      const realClient = getRealClient(client)

-      const { sharepointFileUrl, sheetName, processAllSheets, sheetTableMapping } = input as any
+      const { sharepointFileUrl, sheetTableMapping, processAllSheets } = input


question: Could you clarify what the purpose of processAllSheets is?

ermek-botpress · 2025-11-03T14:30:15Z

integrations/sharepoint-excel/src/index.ts

issue: Please refrain from using nested try/catch blocks

ermek-botpress · 2025-11-03T14:31:54Z

integrations/sharepoint-excel/src/index.ts

suggestion: You can use this function to meaningfully extract error message:

import { isAxiosError } from 'axios' import { ZodError, ZodIssue } from '@botpress/sdk' const formatZodErrors = (issues: ZodIssue[]) => 'Validation Error: ' + issues .map((issue) => { const path = issue.path?.length ? `${issue.path.join('.')}: ` : '' return path ? `${path}${issue.message}` : issue.message }) .join('\n') export const getErrorMessage = (err: unknown): string => { if (isAxiosError(err)) { // server dependent error const status = err.response?.status const data = err.response?.data // always present const message = err.message if (typeof data === 'string' && data.trim()) { return status ? `${data} (Status: ${status})` : data } return status ? `${message} (Status: ${status})` : message } if (err instanceof ZodError) { return formatZodErrors(err.errors) } if (err instanceof Error) { return err.message } if (typeof err === 'string') { return err } if (err && typeof err === 'object' && 'message' in err) { const message = (err as { message: unknown }).message if (typeof message === 'string') { return message } } return 'An unexpected error occurred' }

ermek-botpress · 2025-11-03T14:35:14Z

integrations/sharepoint-excel/src/index.ts

            continue
          }

+          // Check for duplicate headers


issue: This code is hard to follow. Could you refactor it?

ermek-botpress · 2025-11-03T14:37:56Z

integrations/sharepoint-excel/src/index.ts


            if (rowsData.length > 0 && tableId) {
+              logger.forBot().info(`Clearing all existing rows from table "${tableNameForSheet}" (ID: ${tableId}).`)
+              try {


suggestion: Please try to use one try/catch block

ermek-botpress · 2025-11-03T14:39:18Z

integrations/sharepoint-excel/src/index.ts

+                  logger
+                    .forBot()
+                    .warn(
+                      `Could not fetch table schema for "${tableNameForSheet}": ${error.message}. Will use string types for all columns.`


question: Will silent fail prevent user from knowing if tehre is an issue?

ermek-botpress · 2025-11-03T14:39:56Z

integrations/sharepoint-excel/src/index.ts

+
+                  // Log schema changes (only adding columns, not removing)
+                  if (columnsToAdd.length > 0 || columnsToRemove.length > 0) {
+                    logger


question: Should we log or throw errors in these checks?

ermek-botpress · 2025-11-03T14:41:20Z

integrations/sharepoint-excel/src/index.ts

+                    })
+
+                    // Preserve columns that are no longer in Excel (to maintain KB links)
+                    columnsToRemove.forEach((col) => {


issue: This code is very hard to follow. Could you refactor it to make more readable?

- Fix schema not being fetched for newly created tables, causing numeric columns to be converted to strings - Add duplicate column header detection with clear error messages - Add processAllSheets input parameter to control error handling behavior - Update examples to use valid table names ending with 'Table' - Remove unused variables and type casting

- Separate action implementation into dedicated actions/ directory - Add getErrorMessage utility for consistent error handling across all catch blocks - Remove nested try/catch blocks by extracting focused helper functions - Refactor schema update logic into updateTableSchema() function - Refactor row insertion logic into insertTableRows() function - Extract duplicate header validation into validateUniqueHeaders() function - Remove processAllSheets parameter - always continue on errors and return failedSheets[] in output - Make schema fetch operations fail fast instead of using fallback schemas - Fix misleading logs about preserving removed columns (they are actually deleted) - Use `unknown` type for error handling instead of `any`

sham-bolic · 2025-11-10T19:10:34Z

Suggestion: I would ask @SimonNRisk about opting in to using the global pnpm workspace

ptrckbp

syncExcelFile needs to be simplified. There's too much going on in it. It's hard to follow or make sure all is well.

ptrckbp · 2025-11-10T19:45:16Z

integrations/sharepoint-excel/integration.definition.ts

 export default new IntegrationDefinition({
  name: 'plus/sharepoint-excel',
-  version: '2.2.1',
+  version: '2.3.1',


question: would 2.3.0 work?

yup, changed this to 2.3.0

ptrckbp · 2025-11-10T19:45:34Z

integrations/sharepoint-excel/integration.definition.ts

      privateKey: z.string().min(1).describe('PEM‑formatted certificate private key'),
      primaryDomain: z.string().min(1).describe('SharePoint primary domain (e.g. contoso)'),
      siteName: z.string().min(1).describe('SharePoint site name'),
-      personalAccessToken: z.string().min(1).describe('Botpress Personal Access Token (PAT) for Tables API access'),


praise: removing unneeded permissions is great!

ptrckbp · 2025-11-10T19:46:23Z

integrations/sharepoint-excel/integration.definition.ts

            .string()
            .describe(
-              'Map sheets to tables. Format: \'Sheet1:table1,Sheet2:table2\' or JSON: \'{"Sheet1":"table1","Sheet2":"table2"}\''
+              'Map sheets to tables. Format: \'Sheet1:productsTable,Sheet2:ordersTable\' or JSON: \'{"Sheet1":"productsTable","Sheet2":"ordersTable"}\'. Table names must end with \'Table\'.'


question: why was this change made? Is this integration specific to products and orders?

We needed the table at the end of the table name, but I changed this to be more generic.

ptrckbp · 2025-11-10T19:48:05Z

integrations/sharepoint-excel/integration.definition.ts

+                error: z.string(),
+              })
+            )
+            .optional()


question: why remove the optional from processedSheets but not failedSheets? Both could be empty arrays and processed that way (or null / undefined)

Good point, done.

ptrckbp · 2025-11-10T19:55:07Z

integrations/sharepoint-excel/src/actions/sync-excel-file.ts

+        continue
+      }
+
+      const jsonData = xlsx.utils.sheet_to_json(worksheet, { header: 1 })


todo: rename this to jsonArray or something indicating it's a list. this is not intuitive enough.

ptrckbp · 2025-11-10T19:59:15Z

integrations/sharepoint-excel/src/actions/sync-excel-file.ts

+  return rowsToInsert.length
+}
+
+export const syncExcelFile: bp.IntegrationProps['actions']['syncExcelFile'] = async ({


todo: split this function into parts that are easier to read. This is doing too much. It's getting, then looping over data and processing, then batch-sending it to botpress. Clearing tables sometimes.

Agreed, simplified this to be easier to read.

- Split syncExcelFile into focused helper functions (parseSheetTableMapping, ensureTableExists, processSheet) - Standardize documentation examples to use generic table names - Make output schema consistent by always returning both processedSheets and failedSheets arrays

ermek-botpress · 2025-11-17T19:02:39Z

integrations/sharepoint-excel/src/actions/sync-excel-file.ts

+import { getErrorMessage } from '../utils'
+
+type Logger = Parameters<bp.IntegrationProps['actions']['syncExcelFile']>[0]['logger']
+type RealClient = any


issue: Here and below, please refrain from using any unless absolutely necessary

nabilbotpress requested a review from a team as a code owner October 31, 2025 21:30

ermek-botpress reviewed Nov 3, 2025

View reviewed changes

integrations/sharepoint-excel/src/index.ts Outdated

Copy link

Collaborator

ermek-botpress Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Please refrain from using nested try/catch blocks

ermek-botpress reviewed Nov 3, 2025

View reviewed changes

envyrovasileski and others added 7 commits November 8, 2025 17:42

Automatic column addition and deletion

303bdc4

Refine polling times

646c61c

Bump integration version

97f47c6

Directly use client table methods over PAT + TablesAPI

7622e4d

run prettier

1024d1c

nabilbotpress force-pushed the sharepoint-excel-feat branch from b561b0e to aee0435 Compare November 10, 2025 16:42

ptrckbp requested changes Nov 10, 2025

View reviewed changes

nabilbotpress force-pushed the sharepoint-excel-feat branch from cfc4987 to 8fa2736 Compare November 17, 2025 16:50

Napeel added 4 commits November 17, 2025 11:53

prettier

97a467a

Add sharepoint-excel to pnpm workspace

d18cdeb

Update lockfile

6cb9f25

change version

24bbf79

ermek-botpress reviewed Nov 17, 2025

View reviewed changes

Conversation

nabilbotpress commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sham-bolic commented Nov 10, 2025

Uh oh!

ptrckbp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants